Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(autotests): update use of unit_conversion to length_conversion and time_conversion #1282

Merged

Conversation

emorway-usgs
Copy link
Contributor

Relates to #1154

@langevin-usgs
Copy link
Contributor

Could this PR be updated using a rebase instead of a merge. It's hard to tell what changes are unique to this PR.

@emorway-usgs
Copy link
Contributor Author

@langevin-usgs, I'm surprised it is showing up as a merge, I tried rebasing using the following command from a bash console

rebase

I must not have pushed correctly. Sorry about this, I'll try again.

@emorway-usgs emorway-usgs force-pushed the update_unit_conv_to_latest branch from b481c76 to 1b9eb20 Compare July 21, 2023 17:47
@emorway-usgs emorway-usgs force-pushed the update_unit_conv_to_latest branch from 1b9eb20 to 775fdb0 Compare August 7, 2023 20:12
@emorway-usgs emorway-usgs force-pushed the update_unit_conv_to_latest branch from 245d61c to b291ca9 Compare August 8, 2023 17:08
@emorway-usgs
Copy link
Contributor Author

emorway-usgs commented Aug 8, 2023

A note on replacing the baked-in answers for the 4 SFR smoke tests that were updated with length_conversion and time_conversion to clean-up deprecation warnings: Previously, when using unit_conversion in SFR, it was implied that unit_conversion was the product of a length conversion factor and time conversion factor. This was based on how the input instructions were written. However, after #1154, the following was introduced to the SFR source code, inside the function sfr_check_conversion:

this%unitconv = this%unitconv * this%lengthconv**DONETHIRD

Owing to the fact that lengthconv is raised to the 1/3 power, the results forthcoming from the smoke tests will be different when using time_conversion and length_conversion as opposed to unit_conversion.

Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something doesn't seem quite right here. Shouldn't we be able to get the same results for the prudic problem? The simulated concentrations are quite different and don't match any more like they do here. I think we want to produce results that look more or less like this:
image
Or maybe I'm missing something?

@emorway-usgs emorway-usgs force-pushed the update_unit_conv_to_latest branch from b291ca9 to 8ede838 Compare August 9, 2023 19:12
@emorway-usgs
Copy link
Contributor Author

@langevin-usgs, it was me that was missing something. The factor 1.486 already reflected the fact that a cube root was applied to 3.28084, kind of embarrassing I didn't pick up on that before. I should've cubed 1.486 when I switched from unit_conversion to length_conversion originally. It after the autotests complete on f96818f, this should be good to go.

Copy link
Contributor

@langevin-usgs langevin-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Eric, this looks much better. Only one final comment. Should length_conversion be more precisely specified as 3.28084 instead of the value that you use (3.281379)? It doesn't matter for the comparisons.

@emorway-usgs
Copy link
Contributor Author

@langevin-usgs, I think this one is ready to go. I initially just cubed 1.486 with the thinking that's what was needed to get the autotests to pass, but as you point out, that wasn't necessary. They pass with the true length_conversion value of 3.28084.

@langevin-usgs langevin-usgs merged commit bc84c1b into MODFLOW-ORG:develop Aug 15, 2023
@emorway-usgs emorway-usgs deleted the update_unit_conv_to_latest branch August 16, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants